Better Presets - #61

src/Responsive/Descriptor/WDescriptor.php (1)

87-108: Duplicated validation logic with validateModifierValue.

Lines 95–107 repeat the same numeric-check-and-in_array logic found in validateModifierValue (lines 74–84). Consider delegating to validateModifierValue (or extracting a shared private helper) to keep validation consistent in one place.

Proposed refactor

 public function expandModifier(
     ModifierCollectionInterface $modifierCollection,
     mixed $value,
 ): array {
     $wAlias = $modifierCollection
         ->getByName(Width::class)
         ->getAlias();

-    if (is_numeric($value) && in_array((int) $value, $this->widths, true)) {
-        return [
-            $wAlias => (int) $value,
-        ];
-    }
-
-    throw new InvalidArgumentException(
-        message: sprintf(
-            'Invalid preset value "%s" passed for descriptor %s',
-            var_export($value, true),
-            $this,
-        ),
-    );
+    return [
+        $wAlias => $this->validateModifierValue($value, null),
+    ];
 }

Note: validateModifierValue($value, null) with null default works because $value here is expected to be numeric already (not true). If true can be passed in this context, adjust the second argument accordingly.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Responsive/Descriptor/WDescriptor.php` around lines 87 - 108,
expandModifier duplicates the numeric + in_array validation already implemented
in validateModifierValue; remove the duplicated logic and delegate to that
method (or call a new shared private helper) to ensure single-source validation.
Specifically, inside WDescriptor::expandModifier call
$this->validateModifierValue($value, null) (or the extracted helper) and use its
result to build and return the array with the width alias (obtained via
getByName(Width::class)->getAlias()); if validation fails let
validateModifierValue throw the same InvalidArgumentException so expandModifier
no longer repeats the numeric/in_array checks.

tests/Modifier/Preset/PresetCollectionTest.phpt (1)

17-36: Tests properly updated for Preset objects.

The test covers add/has/get and the error case. Consider adding assertions for $descriptor and $defaultDescriptorValue properties on the retrieved Preset to ensure the full object round-trips correctly through the collection.

Example additional assertions

     Assert::same(['w' => 15], $collection->get('a')->modifiers);
+    Assert::null($collection->get('a')->descriptor);
+    Assert::null($collection->get('a')->defaultDescriptorValue);
     Assert::same(['w' => 15, 'pd' => 2.0], $collection->get('b')->modifiers);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/Modifier/Preset/PresetCollectionTest.phpt` around lines 17 - 36, Add
assertions in testPresetsShouldBeAdded to verify that Preset objects are
round-tripped with their descriptor and defaultDescriptorValue intact: after
retrieving each Preset via PresetCollection::get('a') and ::get('b'), assert the
Preset->descriptor equals the expected descriptor (null in the current diff) and
Preset->defaultDescriptorValue equals the expected default (null in the current
diff) so both properties are explicitly checked along with modifiers; update
expected values if different from null.

src/Modifier/Codec/Codec.php (1)

40-47: Duplicate default-assignment logic between modifiersToPath and pathToModifiers.

The assigner/separator default logic (fallback to ':' / ',' when empty) is duplicated in both methods (lines 40–44 here and lines 79–80 in pathToModifiers). Consider extracting a small private helper to keep this in one place.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Modifier/Codec/Codec.php` around lines 40 - 47, The default
assigner/separator fallback logic is duplicated in modifiersToPath and
pathToModifiers; extract a private helper method (e.g., private function
normalizeModifierDelimiters(array $config): array) that reads
$this->config[Config::MODIFIER_ASSIGNER] and Config::MODIFIER_SEPARATOR, applies
the empty() ? ':' / ',' defaults, asserts string types, and returns
['assigner'=>..., 'separator'=>...]; then replace the duplicated blocks in
modifiersToPath and pathToModifiers to call this helper and use the returned
assigner and separator variables.

src/Security/KnownModifiers.php (1)

7-19: LGTM — minor naming note on $list.

The implementation is correct: array<string, true> as a hash-set with O(1) isset lookup is the right approach. list is permitted as a property name since PHP 7.0, as reserved keywords are allowed for property, constant, and method names of classes, interfaces, and traits, so there's no technical issue here. The property name $list is a bit generic though — a more descriptive name like $modifierPaths would better convey its contents.

✏️ Proposed rename for clarity

-    /**
-     * `@param` array<string, true> $list
-     */
-    public function __construct(
-        public readonly array $list,
-    ) {}
+    /**
+     * `@param` array<string, true> $modifierPaths
+     */
+    public function __construct(
+        public readonly array $modifierPaths,
+    ) {}

     public function isKnown(string $modifiers): bool
     {
-        return isset($this->list[$modifiers]);
+        return isset($this->modifierPaths[$modifiers]);
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Security/KnownModifiers.php` around lines 7 - 19, Rename the generic
property $list to a clearer name (e.g., $modifierPaths) in the KnownModifiers
class: update the constructor parameter and promoted property, the public
readonly property declaration, and all usages such as the isKnown(string
$modifiers) method to reference the new name ( KnownModifiers::$modifierPaths
and its constructor promotion ). Ensure the docblock stays accurate
(array<string, true>) and run tests or static analysis to catch any remaining
references to $list.

tests/Modifier/Codec/RuntimeCachedCodecTest.phpt (1)

47-75: Test method names are inverted in order.

testStringValueShouldBeDecodedAndCached2 (line 47) appears before testStringValueShouldBeDecodedAndCached (line 62). The 2 suffix conventionally implies a follow-up test, but here it comes first, which is confusing. Consider renaming them to reflect their actual purpose, e.g. testPathCanBeDecodedAndResultIsCached and testCachedResultIsReturnedWithoutCallingInnerCodecAgain.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/Modifier/Codec/RuntimeCachedCodecTest.phpt` around lines 47 - 75, The
test method names are out of logical order and unclear: rename the two methods
(currently testStringValueShouldBeDecodedAndCached2 and
testStringValueShouldBeDecodedAndCached) to clearer, purpose-revealing names
such as testPathCanBeDecodedAndResultIsCached for the one asserting
decoding+cache behavior and
testCachedResultIsReturnedWithoutCallingInnerCodecAgain for the one asserting
the inner codec is only called once; update both method declarations (and any
references) accordingly so their names reflect intent and order.

README.md (1)

90-99: LGTM — minor indentation inconsistency in the example.

The new nested modifiers: structure and the w/defaultW top-level preset fields are clearly illustrated. One nit: my_preset's modifiers children use 2-space extra indentation (lines 93–94), while my_preset_2's use 4-space extra (line 97). Both are valid NEON, but aligning them aids readability.

📝 Proposed consistency fix

             presets:
                 my_preset:
                     modifiers:
-                      w: 150
-                      ar: '2x1.5'
+                        w: 150
+                        ar: '2x1.5'
                 my_preset_2:
                     modifiers:
                         ar: 1x2
                     w: [300, 600, 900]
                     defaultW: 600

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@README.md` around lines 90 - 99, The README example has inconsistent
indentation for the presets: align the nested modifiers children consistently
between my_preset and my_preset_2 (e.g., make the w and ar under my_preset use
the same 4-space extra indentation style as my_preset_2); update the modifiers
block under my_preset so the w and ar lines match the indentation of my_preset_2
(presets, my_preset, my_preset_2, modifiers, w, ar, defaultW).

src/Modifier/Codec/PresetCodec.php (1)

62-63: Implicit array initialization of $modifiers on line 63.

$modifiers is first used with $modifiers[] = $preset->modifiers without prior declaration. While this is valid PHP that auto-creates the array, an explicit initialization improves readability.

♻️ Optional: explicit initialization

         $preset = $this->presetCollection->get(presetAlias: $presetAlias);
-        $modifiers[] = $preset->modifiers;
+        $modifiers = [$preset->modifiers];

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Modifier/Codec/PresetCodec.php` around lines 62 - 63, The code uses
$modifiers[] = $preset->modifiers without prior declaration; explicitly
initialize $modifiers as an empty array before its first use (e.g., add
$modifiers = [] before the loop or before the line that calls
$this->presetCollection->get) so the variable is declared clearly; update the
function in PresetCodec (where $presetAlias is retrieved via
$this->presetCollection->get) to declare $modifiers = [] prior to appending.

src/LinkGenerator/LinkGenerator.php (2)

49-51: getModifiers() called twice on line 49.

Consider caching the result in a local variable to avoid the redundant call and improve readability.

♻️ Optional: cache modifiers

-        if (null === $pathInfo->getModifiers() || [] === $pathInfo->getModifiers()) {
+        $modifiers = $pathInfo->getModifiers();
+        if (null === $modifiers || [] === $modifiers) {
             $pathInfo = $pathInfo->withModifiers(['original' => true]);
         }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/LinkGenerator/LinkGenerator.php` around lines 49 - 51, The code calls
$pathInfo->getModifiers() twice; cache its result in a local variable (e.g.,
$modifiers = $pathInfo->getModifiers()) and use that variable for the null/empty
check and the subsequent logic so you only invoke getModifiers() once; if
$modifiers is null or an empty array call $pathInfo =
$pathInfo->withModifiers(['original' => true]) as before.

---

101-124: Duplicated assigner-fallback logic across LinkGenerator and PresetCodec.

Lines 107-108 mirror the same pattern in PresetCodec::doExpand (lines 55-56). If the fallback logic changes, both locations need updating.

Consider extracting the assigner resolution (with the empty-string fallback to ':') into a shared utility or Config method to keep it DRY. This is not urgent but worth tracking for a follow-up.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/LinkGenerator/LinkGenerator.php` around lines 101 - 124, The
assigner-fallback logic duplicated in LinkGenerator::resolveDescriptor (using
$this->config[Config::MODIFIER_ASSIGNER] with an empty-string fallback to ':')
should be extracted into a single utility method (e.g.,
Config::getModifierAssigner() or a small helper like
ModifierAssigner::resolve()) and then used from both
LinkGenerator::resolveDescriptor and PresetCodec::doExpand; update
resolveDescriptor to call the new method instead of repeating lines 107-108 and
remove the duplicate logic from PresetCodec so both locations rely on the shared
implementation.

src/Bridge/Nette/DI/ImageStorageExtension.php (1)

619-636: Variable $modifiers on line 628 shadows the outer $modifiers declared on line 584.

Inside the foreach loop, $modifiers = $preset->modifiers overwrites the reference to the modifier instances array built on lines 584–593. While no code after the loop uses the outer $modifiers, this shadowing is a maintenance hazard.

♻️ Rename the inner variable to avoid shadowing

-            $modifiers = $preset->modifiers;
+            $presetModifiers = $preset->modifiers;
 
              foreach ($preset->descriptor->iterateModifiers($modifierCollection) as $mod) {
                  $known[] = $codec->modifiersToPath(array_merge(
-                    $modifiers,
+                    $presetModifiers,
                      $mod,
                  ));
              }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Bridge/Nette/DI/ImageStorageExtension.php` around lines 619 - 636, The
inner variable $modifiers inside the foreach over $presets shadows the outer
$modifiers declared earlier (lines ~584–593); rename the inner variable (e.g.,
to $presetModifiers or $baseModifiers) so it does not overwrite the outer one
and update its use in the array_merge call inside the descriptor loop (the code
that currently does $modifiers = $preset->modifiers and array_merge($modifiers,
$mod) should use the new name), leaving the outer $modifiers untouched.

tests/Modifier/Codec/PresetCodecTest.phpt (1)

23-180: Consider extracting repeated mock setup into a helper or setUp.

Every test method repeats the same four-mock + constructor boilerplate. A setUp() method (or a private factory) would reduce duplication and make the individual tests focus on the scenario-specific expectations.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/Modifier/Codec/PresetCodecTest.phpt` around lines 23 - 180, Extract the
repeated mock and constructor boilerplate by adding a setUp() in the test class
that creates and assigns properties for the common mocks and SUT (e.g.
$this->innerCodec, $this->config, $this->modifierCollection,
$this->presetCollection and $this->presetCodec = new PresetCodec(...)); update
each test to reuse those properties and only set scenario-specific expectations
(e.g. calls to $this->config->shouldReceive(...),
$this->presetCollection->shouldReceive(...),
$this->innerCodec->shouldReceive(...)); alternatively implement a private
factory method (e.g. createPresetCodec()) that returns the configured mocks and
PresetCodec instance and have tests call that to remove duplication.

src/Modifier/Codec/RuntimeCachedCodec.php (1)

32-37: Cache key for modifiersToPath is order-sensitive on array inputs.

json_encode produces different strings for ['w' => 100, 'h' => 200] vs ['h' => 200, 'w' => 100], leading to cache misses for semantically equivalent modifier sets. If callers consistently produce arrays in the same insertion order this is a non-issue, but worth being aware of.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Modifier/Codec/RuntimeCachedCodec.php` around lines 32 - 37, The cache
key for RuntimeCachedCodec::modifiersToPath is order-sensitive because it
json_encodes arrays; normalize array inputs into a canonical, order-independent
representation before encoding by recursively sorting associative array keys
(leave scalar/string inputs as-is), then json_encode that normalized value for
use as the $key when reading/writing $this->cache['modifiersToPath']; after
normalization call $this->codec->modifiersToPath($value) as before.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

src/ImageServer/LocalImageServer.php (1)

151-151: ⚠️ Potential issue | 🟠 Major

# @phpstan-ignore-line`` suppresses the error but does not fix the runtime type hazard.

The array|string|null return type of getQueryParameter is still in play: ?? '' coalesces only null, so an array value (e.g. ?_s[]=foo) passes straight to (string), emitting a Notice: Array to string conversion and producing the literal token "Array". This was flagged in a previous review; the @phpstan-ignore-line annotation silences static analysis but leaves the runtime behaviour unchanged.

🛡️ Proposed fix (from previous review)

-        $token = (string) ($request->getQueryParameter($signatureParameterName) ?? ''); # `@phpstan-ignore-line`
+        $rawToken = $request->getQueryParameter($signatureParameterName);
+        $token = is_string($rawToken) ? $rawToken : '';

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ImageServer/LocalImageServer.php` at line 151, The current assignment to
$token in LocalImageServer.php blindly casts the result of
$request->getQueryParameter($signatureParameterName) to string and suppresses
static analysis; instead, handle array returns explicitly: retrieve $param =
$request->getQueryParameter($signatureParameterName), check is_array($param) and
pick a safe scalar (e.g. $param[0] ?? ''), otherwise use the scalar or null
default, then cast to string and remove the `@phpstan-ignore-line`; update the
code around the $token assignment in the LocalImageServer class/method to use
this safe branching.

tests/Modifier/Facade/ModifierFacadeTest.phpt (1)

169-183: Missing test for the new getPresetCollection() accessor.

testModifierCollectionShouldBeReturned and testCodecShouldBeReturned both follow the same trivial pattern — getPresetCollection() is the only new public method on this facade without equivalent coverage.

💡 Suggested test to add

public function testPresetCollectionShouldBeReturned(): void
{
    $presetCollection = Mockery::mock(PresetCollectionInterface::class);
    $facade = $this->createModifierFacade(presetCollection: $presetCollection);

    Assert::same($presetCollection, $facade->getPresetCollection());
}

Would you like me to open a new issue to track this?

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/Modifier/Facade/ModifierFacadeTest.phpt` around lines 169 - 183, Add a
unit test for the new getPresetCollection() accessor: create a Mockery mock of
PresetCollectionInterface, pass it into createModifierFacade via the
presetCollection parameter, then assert the facade->getPresetCollection()
returns the same mock (similar pattern to testModifierCollectionShouldBeReturned
and testCodecShouldBeReturned). Ensure the test method is named
testPresetCollectionShouldBeReturned and uses Assert::same to compare the
returned value to the provided mock.

src/Modifier/Facade/ModifierFacade.php (1)

104-107: Consider using $this->codec directly for consistency.

Every other private-field access in this class is direct ($this->modifierCollection, $this->config, etc.). Routing through the public getter is inconsistent inside a final class.

♻️ Proposed refactor

-        if (is_string($modifiers)) {
-            $codec = $this->getCodec();
-            $modifiers = $codec->expandModifiers(value: $modifiers);
-        }
+        if (is_string($modifiers)) {
+            $modifiers = $this->codec->expandModifiers($modifiers);
+        }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Modifier/Facade/ModifierFacade.php` around lines 104 - 107, Replace the
indirect getter call with direct private-field access for consistency: inside
the ModifierFacade method containing the snippet (in the final class
ModifierFacade), change uses of $this->getCodec() to $this->codec so the block
becomes $codec = $this->codec; (and keep the subsequent
$codec->expandModifiers(...) logic unchanged); this aligns with other
private-field accesses like $this->modifierCollection and $this->config while
preserving behavior.

Configuration used: defaults

Review profile: CHILL

Plan: Pro